Conversation
WalkthroughThis PR adds a public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/source/lob_spin_drift_test.cpp (2)
25-30: Fixture constants could bestatic constexpr.These constants are identical across all test instances. Declaring them as
static constexprwould be slightly more efficient and clearer in intent:- const double kZeroDistance = 300; - const double kOgiveLength = 0.748; - const double kTailLength = 0.257; - const double kMeplatDiameter = 0.069; - const double kBaseDiameter = 0.276; - const double kRtR = 0.99; + static constexpr double kZeroDistance = 300; + static constexpr double kOgiveLength = 0.748; + static constexpr double kTailLength = 0.257; + static constexpr double kMeplatDiameter = 0.069; + static constexpr double kBaseDiameter = 0.276; + static constexpr double kRtR = 0.99;
114-135: Consider extracting common verification logic.The verification loop pattern is repeated identically across 9+ tests. While inline assertions are often preferred in tests for clearer failure messages, this level of duplication could be reduced with a helper function:
void VerifySolutions(const std::array<lob::Output, N>& solutions, const std::vector<lob::Output>& expected, uint16_t velocity_error, uint16_t energy_error, double moa_error, double inch_error, double tof_error);This is optional since the current approach provides explicit, readable test output on failures.
test/source/lob_cwaj_test.cpp (2)
114-135: Consider extracting the validation loop into a helper function.The solution validation pattern (comparing range, velocity, energy, elevation MOA/inches, deflection MOA/inches, time_of_flight) is repeated across 10+ tests. A helper like
ExpectSolutionNear(solutions, expected, tolerances)would reduce duplication and centralize tolerance definitions.Example approach:
void ExpectSolutionsNear( const std::span<const lob::Output> solutions, const std::span<const lob::Output> expected, uint16_t velocity_err, uint16_t energy_err, double moa_err, double inch_err, double tof_err) { ASSERT_EQ(solutions.size(), expected.size()); for (size_t i = 0; i < solutions.size(); ++i) { EXPECT_EQ(solutions[i].range, expected[i].range); EXPECT_NEAR(solutions[i].velocity, expected[i].velocity, velocity_err); // ... remaining assertions } }
740-741: Verify if the commented-out kSMK168 test case should be tracked.This test case is excluded but the reason isn't documented. Consider adding a comment explaining why it's disabled, or creating an issue to investigate if it should be enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CODE_OF_CONDUCT.md(1 hunks)README.md(2 hunks)include/lob/lob.hpp(1 hunks)source/lob_builder.cpp(2 hunks)test/source/lob_cwaj_test.cpp(11 hunks)test/source/lob_spin_drift_test.cpp(11 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-15T13:11:22.842Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 15
File: source/calc.hpp:52-52
Timestamp: 2025-04-15T13:11:22.842Z
Learning: In lob, the `CalculateProjectileReferenceArea` function returns area in square inches (SqInT), but in some calculations like those in test/source/calc_test.cpp, this value is intentionally converted to square feet (SqFtT) when multiplied by air density in pounds per cubic foot (LbsPerCuFtT) to ensure proper unit cancellation.
Applied to files:
test/source/lob_spin_drift_test.cppREADME.mdtest/source/lob_cwaj_test.cpp
📚 Learning: 2025-06-17T12:55:26.522Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 25
File: include/lob/lob.hpp:123-130
Timestamp: 2025-06-17T12:55:26.522Z
Learning: In the lob ballistics library, when Input::step_size is set to 0 (the default), the solver automatically sets an appropriate step size internally rather than failing with an assertion error.
Applied to files:
test/source/lob_spin_drift_test.cppREADME.mdtest/source/lob_cwaj_test.cpp
📚 Learning: 2025-05-22T16:12:16.630Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: test/source/boatright_test.cpp:58-65
Timestamp: 2025-05-22T16:12:16.630Z
Learning: In the lob codebase, std::pow and potentially other standard library functions have been overloaded to work directly with strong types (like InchT, CaliberT, etc.), allowing expressions like std::pow(kD / 2, 2) to compile correctly without needing to extract the underlying value first.
Applied to files:
README.md
📚 Learning: 2025-05-23T12:16:39.175Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: source/boatright.cpp:72-81
Timestamp: 2025-05-23T12:16:39.175Z
Learning: In the lob codebase, std::pow has been overloaded in eng_units.hpp to work directly with strong unit types (like InchT, CaliberT, etc.), allowing expressions like std::pow(diameter / 2, 2) to compile correctly without needing to extract raw values using the Value() method.
Applied to files:
README.md
📚 Learning: 2025-05-23T20:47:45.448Z
Learnt from: joelbenway
Repo: joelbenway/lob PR: 22
File: source/lob_solve.cpp:0-0
Timestamp: 2025-05-23T20:47:45.448Z
Learning: In lob_solve.cpp, the loop pattern that accesses pranges[index] is safe from out-of-bounds access because it checks if (index >= size) and breaks immediately after incrementing index, ensuring pranges[index] is never accessed with an invalid index.
Applied to files:
test/source/lob_cwaj_test.cpp
🧬 Code graph analysis (2)
test/source/lob_spin_drift_test.cpp (1)
source/lob_convert.cpp (2)
InchToMoa(39-44)InchToMoa(39-39)
source/lob_builder.cpp (1)
include/lob/lob.hpp (1)
IsValid(445-445)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (ubuntu-latest, shared)
- GitHub Check: test (ubuntu-latest, static)
- GitHub Check: test (windows-latest, static)
- GitHub Check: test (windows-latest, shared)
- GitHub Check: test (macos-latest, shared)
- GitHub Check: test (macos-latest, static)
- GitHub Check: sanitize
- GitHub Check: coverage
🔇 Additional comments (14)
README.md (1)
28-28: LGTM!The emoji updates are consistent throughout the document—replacing the US flag references with statue of liberty and eagle emojis.
Also applies to: 139-139
CODE_OF_CONDUCT.md (1)
9-20: LGTM!The bullet list formatting is now cleaner and more consistent without trailing commas.
include/lob/lob.hpp (1)
441-445: LGTM!The new
IsValid()public API is well-documented and properly declared asconst. This provides a useful validation query before callingBuild().source/lob_builder.cpp (2)
432-441: LGTM!The
IsValid()implementation correctly validates the minimum required inputs:
- Error state is acceptable (not a previous validation failure)
- Ballistic coefficient is set
- Velocity is set (non-zero)
- Either zero distance or zero angle is provided
This aligns with the minimal example in the README where only BC, velocity, and zero distance are required.
783-805: LGTM!The
Build()integration withIsValid()is well-structured:
- Resets error state to
kNotFormedif previously successful (allows re-building)- Validates via
IsValid()before proceeding- Executes build steps in correct dependency order
- Marks error as
kNoneon successful completiontest/source/lob_spin_drift_test.cpp (2)
8-8: LGTM!The
<cmath>include is necessary forstd::isnan()used in the spindrift_factor validation assertions.
370-432: Comprehensive Boatright model test coverage.The new Boatright tests appropriately:
- Configure all required ogive parameters (nose length, tail length, base diameter, meplat diameter, RtR)
- Verify
spindrift_factoris non-NaN (confirming Boatright path was taken)- Test both right-hand and left-hand twist with standard and fast twist rates
The distinction between Litz (NaN spindrift_factor) and Boatright (non-NaN spindrift_factor) tests provides clear validation that the correct calculation path is being exercised.
test/source/lob_cwaj_test.cpp (7)
25-31: LGTM: Centralized fixture constants improve test maintainability.Good refactoring to extract these shared parameters as fixture-level constants, ensuring consistency across all spin/wind scenario tests.
38-43: Well-documented test data source.Citing the reference (Litz, pg 656) makes it easier to verify expected values and understand the test scenario.
64-72: LGTM!Using the fixture constant
kZeroDistanceensures consistency with the other tests.
83-136: Test data looks physically consistent.The SolveWithoutSpin test correctly shows zero deflection at all ranges (no spin drift without twist parameter), with expected velocity decay and trajectory drop. The validation loop thoroughly checks all solution attributes.
139-199: Litz model tests demonstrate correct symmetry.The aerodynamic jump values (±0.650208) correctly flip signs for opposite twist directions. Deflection values appropriately become more negative with left-hand spin in leftward wind conditions.
392-458: LGTM: Boatright model tests properly utilize extended bullet geometry.The additional parameters (
NoseLengthInch,TailLengthInch,BaseDiameterInch,MeplatDiameterInch,OgiveRtR) correctly enable the Boatright aerodynamic jump calculation, producing a higher magnitude (1.015683 vs 0.650208 for Litz) as expected from the more detailed model.
695-712: LGTM!The parameterized Boatright test correctly validates the aerodynamic jump calculation against expected values with 10% tolerance, accounting for variations in the model.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.